Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support loading RSAPSS public keys with parameters #268

Merged
merged 7 commits into from
Oct 3, 2024

Conversation

gautierdelorme
Copy link
Contributor

Support loading RSA PSS public keys with parameters.

Checklist

  • I've run tests to see all new and existing tests pass
  • I've followed the code style of the rest of the project
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary

If you've made changes to gyb files

  • I've run .script/generate_boilerplate_files_with_gyb and included updated generated files in a commit of this pull request

Motivation:

Trying to load those keys currently fails.

Modifications:

Parameters are stripped and the key is treated as a regular public key.

Result:

_RSA.Signing.PublicKey() works with RSA PSS keys with parameters.

Parameters are stripped and the key is treated as a regular public key.
@Lukasa Lukasa added the semver/patch No public API change. label Oct 3, 2024
@Lukasa
Copy link
Collaborator

Lukasa commented Oct 3, 2024

@swift-server-bot add to allowlist

Package.swift Outdated
@@ -84,7 +84,9 @@ let package = Package(
.library(name: "CCryptoBoringSSL", type: .static, targets: ["CCryptoBoringSSL"]),
MANGLE_END */
],
dependencies: [],
dependencies: [
.package(url: "https://github.com/apple/swift-asn1.git", .upToNextMajor(from: "1.2.0"))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

Suggested change
.package(url: "https://github.com/apple/swift-asn1.git", .upToNextMajor(from: "1.2.0"))
.package(url: "https://github.com/apple/swift-asn1.git", from: "1.2.0")

//
//
// Created by Gautier Delorme on 24/09/2024.
//
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add the license header that is present in other files and remove this header?

/// - Warning: Key sizes less than 2048 are not recommended and should only be used for compatibility reasons.
public init<Bytes: DataProtocol>(unsafeDERRepresentation derRepresentation: Bytes) throws {
self.backing = try BackingPublicKey(derRepresentation: derRepresentation)
let sanitizedDer = try SubjectPublicKeyInfo.stripRsaPssParameters(derEncoded: [UInt8](derRepresentation))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a request for changes in this PR, but can you file an issue that asks us to move all our RSA key parsing into Swift ASN1? This is a pragmatic change for now, but as we're already parsing the key we should probably just stick with that now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sources/_CryptoExtras/Util/SubjectPublicKeyInfo.swift Outdated Show resolved Hide resolved
@Lukasa
Copy link
Collaborator

Lukasa commented Oct 3, 2024

Thanks for this PR! I've left a few notes in the diff for cleaning up.

return derEncoded
}

if spki.algorithmIdentifier.algorithm == .AlgorithmIdentifier.rsaPSS {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we turn this into a shortcut early return? If we don't have to make any modifications, we don't need to re-encode the key.

Copy link
Collaborator

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, this LGTM.

@Lukasa
Copy link
Collaborator

Lukasa commented Oct 3, 2024

Thanks very much!

@Lukasa Lukasa enabled auto-merge (squash) October 3, 2024 17:23
auto-merge was automatically disabled October 3, 2024 17:23

Head branch was pushed to by a user without write access

@Lukasa Lukasa merged commit b639b5b into apple:main Oct 3, 2024
8 of 9 checks passed
@gautierdelorme gautierdelorme deleted the gdelorme/rsapss branch October 3, 2024 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants